Expired shard's directory no longer getting deleted since 0.8.4? #1078
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Background of the bug: Prior to this patch we actually tried writing points that were older than the retention period of the shard. This caused race condition when it came to writing points to a shard that's being dropped, which will happen frequently if the user is loading old data (by accident). This is demonstrated in the test in this commit.This bug was previously addressed in #985. It turns the fix for #985 wasn't enough. A user reported in #1078 that some shards are left behind and not deleted. It turns out that while the shard is being dropped more write requests could come in and end up on line `cluster/shard.go:195` which will cause the datastore to create a shard on disk that isn't tracked anywhere in the metadata. This shard will live forever and never get deleted. This fix address this issue by not writing old points in, but there are still some edge cases with the current implementation, at least not as bad as current master.
|
@jvshahid the fix lgtm. I think the |
| @@ -507,20 +507,31 @@ func (self *Coordinator) CommitSeriesData(db string, serieses []*protocol.Series | |||
| // TODO: this isn't needed anymore | |||
| series.SortPointsTimeDescending() | |||
|
|
|||
| sn := series.GetName() | |||
| // use regular for loop since we update the iteration index `i' as | |||
| // we batch points that have the same timestamp | |||
| for i := 0; i < len(series.Points); { | |||
There was a problem hiding this comment.
In a separate issue, I think we should refactor this loop to use a ranged loop and pull some things out of the loop.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since we upgraded from 0.8.3 to 0.8.4 (and subsequently to 0.8.5), it looks like InfluxDB no longer deletes the directories containing expired shards' data. This is the log output from around the latest shard rollover which took place today at 01:00 CET:
At this time, older shard number 61 expired and was actually removed from the configuration (i.e. it's no longer visible in the admin GUI), but as opposed to the behavior we had in 0.8.3, the directory containing the RocksDB data didn't get removed.